esq5505.cpp: Add VFX-family ROM & EEPROM Cartridge support, and improve floppy support. #14444

cbrunschen · 2025-10-31T18:38:54Z

Second attempt, simpler and hopefully better.

I've tried to address the issues identified in the Revert commit message. Specifically, the cartridge is now represented by a single image device class, and vfxcart.h includes only diimage.h, not emu.h. All indentation should also now be tabs, not spaces, and there should be no trailing spaces (running srcclean changes nothing further).

cbrunschen · 2025-10-31T22:03:52Z

While I've tried to address the issues that were raised when the previous attempt at this was reverted, I would appreciate feedback on what I have missed, even in small things.

…ve floppy support. Second attempt, simpler and hopefully better.

… indicate this on the LED on the panel layout.

MooglyGuy

Looks reasonable to me other than some minor suggestions/requests. Hopefully @rb6502 or @galibert can take the time to give it a look-see as well.

MooglyGuy · 2025-11-05T17:39:39Z

src/mame/ensoniq/esq5505.cpp

+
+void esq5505_state::cartridge_load(ensoniq_vfx_cartridge *cart)
+{
+	(void) cart;


No need to explicitly void out unused function parameters, I believe any warnings that could be caused have been suppressed since forever.

Habit from other C++ code I work on. Removed.

MooglyGuy · 2025-11-05T17:41:59Z

src/mame/ensoniq/esq5505.cpp

+	// update the "Disk Ready" input
+	int state = (m_floppy_is_active && m_floppy_is_loaded) ? ASSERT_LINE : CLEAR_LINE;
+#if VERBOSE
+	static int prev_state = 0;


I would use LOGMASKED for this rather than #if-ing it. #if'd-out code tends to rot. Should be plenty of examples to choose from when it comes to LOGMASKED, the only thing to know is that LOG(...) is effectively an alias for LOGMASKED(LOG_GENERAL, ...), so user-defined verbose masks should start at 1U << 1.

The #ifdef here is also guarding the static int prev_state, which is only there to track the previous state, so the logging doesn't spam non-changing updates; so it's not as straightforward as just replacing LOG with LOGMASKED.
But as this LOG was really only useful during development, I'm removing it; it's simple enough to add something similar back in the future if the need arises.

MooglyGuy · 2025-11-05T18:12:23Z

src/mame/ensoniq/esq5505.cpp

+
+void esq5505_state::floppy_load(floppy_image_device *floppy)
+{
+	(void) floppy;


Same as above, don't need to explicitly void out unused function parameters.

MooglyGuy · 2025-11-05T18:12:58Z

src/mame/ensoniq/esq5505.cpp

+void esq5505_state::update_docirq_to_maincpu()
+{
+	bool floppy_dskchg_irq = m_floppy_is_active && m_floppy_dskchg;
+	if (floppy_dskchg_irq) LOG("docirq (m68k_irq1) due to disk change = %d\n", floppy_dskchg_irq);


MAME isn't super-consistent all the time about this sort of thing, but it would be slightly more preferred to put this on the following line.

MooglyGuy · 2025-11-05T18:13:31Z

src/mame/ensoniq/esq5505.cpp

+		LOG("Scheduling DSKCHG reset\n");
+		m_dskchg_reset_timer->adjust(attotime::from_nsec(500));
+	}
+	bool v = m_otis_irq || floppy_dskchg_irq;


updated_irq instead of v, perhaps?

MooglyGuy · 2025-11-05T18:13:46Z

src/mame/ensoniq/esq5505.cpp

+	}
+	else
+	{
+		// LOG("m68k irq1 -- %d\n", v);


Commented code can be deleted, or if you don't want it logged all the time, give it a separate bitfield and use LOGMASKED.

MooglyGuy · 2025-11-05T18:14:29Z

src/mame/ensoniq/esq5505.cpp

+	}
+	else
+	{
+		// LOG("otis_irq -- %d\n", irq);


Two more contenders for LOGMASKED here.

MooglyGuy · 2025-11-05T18:16:51Z

src/mame/ensoniq/vfxcart.cpp

+
+void ensoniq_vfx_cartridge::write(offs_t offset, u8 data) {
+	if (!m_is_writable) {
+		if (offset > 0x7f00) LOG("!W %04x  (%02x)\r\n", offset, data);


Suggest newline here and on the one above.

MooglyGuy · 2025-11-05T18:17:46Z

src/mame/ensoniq/vfxcart.cpp

+			break;
+
+		case state::WR:
+			if (offset > 0x7f00) LOG("W %04x :  %02x\r\n", offset, data);


MooglyGuy · 2025-11-05T18:19:01Z

src/mame/ensoniq/vfxcart.cpp

+	// read-only cartridge makes no sense!
+	// This allows users to create a cartridge image with a ".rom" or ".cart" extension, write to it _once_,
+	// in this session, until it is unloaded; and whenever in the future they load it again, it will be
+	// detected as ROM (_not_ EEPROM) by its file extension.


Is this behavior accurate to a real VFX (i.e., write-once cartridges)?

No, I have never encountered a write-once cartridge.

The idea is simply one of usability in a simulated environment: this lets you create a read-only cartridge (.cart or rom), but as an empty read-only cartridge makes little sense, it allows writing to it once; otherwise you'll have to first create it as a writable cartridge, then eject it and rename it (from .eeprom or .sc32 to .cart or .rom). This way, you can just create it, with the intention that it will be a read-only cartridge once it has been written to; and without running the risk of forgetting to rename the cartridge, loading it and accidentally overwriting.

I changed this so that the vfxcart device will only create .eeprom or .sc32 images, and will return a suitable error otherwise.

However, I note that MAME still creates the files, leaving them in place and empty.

So the user still has the option to create any given extension supported by the image device - .rom, .cart, .eeprom or .sc32 - but only .eeprom or .sc32 images are writable?

I'm admittedly unsure if there's a good workaround for this.

IMO, there isn't so much of a need for any differentiation in function like that. Support creation and writing to any of the above. Items that exist in a softlist have their underlying data inherently static, with only diffs against the fundamental data being stored, and in a separate file at that. Even if a user is able to write to a ROM-only cartridge, that's about the best one can do in the current implementation.

I'd like to hear from @cuavas or whoever rejected the initial implementation, because this seems like the exact sort of situation that calls for a cartslot-type bus device which supports two different device_cartrom_slot_interfaces, one that bounces out to an image device, and one that only does straight loading of ROM data. Per @cbrunschen, the original PR was rejected for being too complicated, but I'm not sure I would agree, given the two distinct types of pluggable cartridges.

As far as the UX of the VFX goes, users could buy blank, writable cartridges to make their own patches, or they could be non-writable cartridges preloaded with patches. It seems like a perfect fit for the intention behind the original PR, whereas going with a "pure" imagedev just results in annoying hoop-jumping on @cbrunschen's part.

The user can always rename a writable (.eeprom, .sc32) image to .rom or .cart to mark it as read-only.

cbrunschen marked this pull request as ready for review October 31, 2025 22:01

cbrunschen added 2 commits November 5, 2025 13:30

esq5505.cpp: Add VFX-family ROM & EEPROM Cartridge support, and impro…

6897837

…ve floppy support. Second attempt, simpler and hopefully better.

When the floppy is active (i.e., when the motor is actually on), also…

0fb58f4

… indicate this on the LED on the panel layout.

cbrunschen force-pushed the vfx_cart_and_floppy branch from f2ee5f5 to 0fb58f4 Compare November 5, 2025 13:53

MooglyGuy requested changes Nov 5, 2025

View reviewed changes

cbrunschen added 2 commits November 5, 2025 18:38

Address the first round of comments.

9d13988

Only create writeable cartridge images.

c1ec150

The user can always rename a writable (.eeprom, .sc32) image to .rom or .cart to mark it as read-only.

esq5505.cpp: Add VFX-family ROM & EEPROM Cartridge support, and improve floppy support. #14444

Are you sure you want to change the base?

esq5505.cpp: Add VFX-family ROM & EEPROM Cartridge support, and improve floppy support. #14444

Conversation

cbrunschen commented Oct 31, 2025

Uh oh!

cbrunschen commented Oct 31, 2025

Uh oh!

MooglyGuy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants